Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

[IME][Safari] Fix IME composistion broken issue in empty paragraph in Safari. #1783

Merged
merged 1 commit into from
Sep 9, 2019

Conversation

farthinker
Copy link
Contributor

See https://github.com/ckeditor/ckeditor5-engine/issues/1782, ckeditor/ckeditor5-typing#218 and ckeditor/ckeditor5#1333 for details about the issue.

This fix make sure DOM selection untouched while composing. I've tested it with several IMEs, and it worked. But I need some help to confirm it won't break something else. @Reinmar @jodator @Mgsy

@jodator
Copy link
Contributor

jodator commented Aug 14, 2019

Hi, thanks for the updated solution. We will try to check this as soon as possible but it can take a few days due to the holiday season and upcoming release.

The trick with pausing rendering in some cases recurs on various occasions so I'd ping @pjasiun to look at this briefly. (About pausing the renderer during text composing) Do you see any problems with that? Collaboration maybe?

@jodator
Copy link
Contributor

jodator commented Aug 14, 2019

I mark this PR as a sub-PR for: ckeditor/ckeditor5-typing#218

@jodator jodator added the pr:sub label Aug 14, 2019
@pjasiun
Copy link

pjasiun commented Aug 14, 2019

I see it only touches the selection, so it could be fine. But since renderer is very tricky this change needs to be tested deeply.

@jodator
Copy link
Contributor

jodator commented Aug 14, 2019

OK so please take another round of testing of this @Mgsy with both PRs (this & ckeditor/ckeditor5-typing#218)

@Mgsy Mgsy self-requested a review August 19, 2019 07:54
@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2019

Actually, we already did approach it in the past. It's crystal clear that we must not render the selection during composition. There's even a PR which, from its title, was meant to try the same (stop selection rendering): #861. But when I checked the code it actually went much deeper and tried to disable all rendering (which we cannot do). Most likely that's why we never merged it.

So, I agree with @pjasiun – stoping rendering selection should be fine, but it requires some more testing. However, I'd actually prefer if we tested first this change alone to see what it fixes/breaks. Then, when we have a better understanding about it, we can see if we need more changes.

@farthinker, did you work on these changes (#1783 and ckeditor/ckeditor5-typing#218) separately? I mean – do you know perhaps if the current PR is fixing itself some issues? It's much simpler than the other one and it makes it much easier to close it faster. The one in ckeditor5-typing may take more time because I can see it touches a lot more code.

@farthinker
Copy link
Contributor Author

@Reinmar I think ckeditor/ckeditor5-typing#218 should be closed in favor of this PR. I've tried to fix the issue in ckeditor5-typing, but it seems impossible.

Copy link
Member

@Mgsy Mgsy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this PR and it fixes the problem. Also, I didn't find any regression.

@jodator
Copy link
Contributor

jodator commented Sep 9, 2019

@f1ames Sorry for the delay with the review but the topic is not an easy one. So we had to be sure that it will be OK. We'll be merging this soon.

@Reinmar I've check the tests and I don't see that we could do any better tests for composition. I'll add some small stylistic changes to the comments. But It's OK for me so I'll be merging this.

Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good 👍.

jodator added a commit that referenced this pull request Sep 9, 2019
Fix: The renderer shouold not update DOM selection when document has active composition. Closes #1782. Closes ckeditor/ckeditor5#1333.
@jodator jodator merged commit 82e29ad into ckeditor:master Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants